ACP-77: Current validators API for SoV #3404
Conversation
Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
Signed-off-by: Ceyhun Onur <ceyhun.onur@avalabs.org>
Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
…hego into refactor-uptime-manager
ceyonur
changed the title
…urrent-validators-api
ceyonur
added
DO NOT MERGE
snow/validators/traced_state.go
Outdated
| ctx context.Context, | ||
| subnetID ids.ID, | ||
| ) (map[ids.ID]*GetCurrentValidatorOutput, uint64, bool, error) { | ||
| ctx, span := s.tracer.Start(ctx, s.getValidatorSetTag, oteltrace.WithAttributes( |
There was a problem hiding this comment.
Should we fix this in this PR? Seems simple enough to do so.
| message Validator { | ||
| bytes node_id = 1; | ||
| uint64 weight = 2; | ||
| bytes public_key = 3; | ||
| } | ||
|
|
||
| message CurrentValidator { |
There was a problem hiding this comment.
Possibly dumb question but is there a good reason for not adding new fields to the existing Validator message instead of adding a new one? i.e. do we see the data returned by them becoming incompatible at some point? It seems like it should be compatible since protobuf fields are all optional
There was a problem hiding this comment.
It would mean a breaking change. Changing Validator would require changes to related functions + clients. I think if we want to we can deprecate old methods and types in favor of CurrentValidator. @StephenButtolph
There was a problem hiding this comment.
Why not just have a field of type Validator in CurrentValidator? then you don't need to copy the first three fields.
|
My main question with this also in conjunction with ava-labs/subnet-evm#1263, is whether there is a possibility that the SoV validator would share the same nodeID as the ones initially set? |
Both this API and Subnet-EVM relies on Validation ID rather than NodeID. The case where different Validation IDs mapped to same NodeID should already be handled by the state (here), so this API should not return duplicated ones. For the Subnet-EVM we first handle deletions when we handle the new set from P-Chain. It guarantees that we delete the uptime from the state, even if the same nodeID is being deleted + readded under different validationIDs in the same load period. After deletion we add new data with the same nodeID. See here |
…s/avalanchego into test-only-current-validators-api
| uint64 weight = 2; | ||
| bytes public_key = 3; | ||
| uint64 start_time = 4; | ||
| uint64 set_weight_nonce = 5; |
There was a problem hiding this comment.
what is set_weight_nonce for?
There was a problem hiding this comment.
it's the MinNonce of L1 validators
| IsSov: vdr.IsSoV, | ||
| } | ||
| if vdr.PublicKey != nil { | ||
| // This is a performance optimization to avoid the cost of compression |
There was a problem hiding this comment.
serializing in compressed form is cheap. It is the de-compression that is expensive.
There was a problem hiding this comment.
literally copied from L73
There was a problem hiding this comment.
Understood.
Though I think that this comment is a bit misleading - what it's probably actually trying to say is that in order to avoid the expensive decompression in the client side, the server side is not compressing.
| @@ -831,6 +833,64 @@ func (s *state) DeleteExpiry(entry ExpiryEntry) { | |||
| s.expiryDiff.DeleteExpiry(entry) | |||
| } | |||
|
|
|||
| func (s *state) GetCurrentValidatorSet(ctx context.Context, subnetID ids.ID) (map[ids.ID]*validators.GetCurrentValidatorOutput, uint64, error) { | |||
| result := make(map[ids.ID]*validators.GetCurrentValidatorOutput) | |||
There was a problem hiding this comment.
would it make sense to check currentStakers is not nil?
There was a problem hiding this comment.
currentStakers are initialized to be empty in New
This pull request introduces a new gRPC method
GetCurrentValidatorSetto theValidatorStateservice, allowing clients to retrieve the current validator set for a given subnet. The changes span multiple files to implement this new functionality, including updates to the protobuf definitions, client and server interfaces, and associated tests.Additions to
ValidatorStateService:proto/pb/validatorstate/validator_state_grpc.pb.go: Added theGetCurrentValidatorSetmethod to both client and server interfaces, along with its handler and full method name constant. [1] [2] [3] [4] [5] [6] [7]proto/validatorstate/validator_state.proto: Defined theGetCurrentValidatorSetRequestandGetCurrentValidatorSetResponsemessages, and added the new RPC method to theValidatorStateservice. [1] [2]Client and Server Implementation:
snow/validators/gvalidators/validator_state_client.go: Implemented theGetCurrentValidatorSetmethod in theClientstruct to call the new gRPC method.snow/validators/gvalidators/validator_state_server.go: Implemented theGetCurrentValidatorSetmethod in theServerstruct to handle incoming requests.State Interface and Mock Updates:
snow/validators/state.go: Added theGetCurrentValidatorSetmethod to theStateinterface and implemented it in thelockedStatestruct. [1] [2] [3]snow/validators/traced_state.go: Added tracing support for theGetCurrentValidatorSetmethod.snow/validators/validator.go: Defined theGetCurrentValidatorOutputstruct to represent the current validator data.snow/validators/validatorsmock/state.go: Added mock implementations for theGetCurrentValidatorSetmethod.snow/validators/validatorstest/state.go: Added test support for theGetCurrentValidatorSetmethod. [1] [2]Platform VM State Changes:
vms/platformvm/state/state.go: Added theGetCurrentValidatorSetmethod to theStateinterface and implemented it in thestatestruct, including logic to determine if the subnet is L1 and fetch the current validators. [1] [2]vms/platformvm/state/mock_state.go: Added mock implementations for theGetCurrentValidatorSetmethod.